gh-48 : fix add-as-submodule with deeper paths#49
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded runtime Git-repo validation and hardened per-test logging/working-directory handling in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
_tests.sh (1)
447-453:⚠️ Potential issue | 🟡 Minor
okis printed even when a testcase fails.Line 452 is outside the success path, so failures still emit
ok, which is misleading for humans/parsers.🔧 Proposed fix
- for fn in "${test_functions[@]}"; do - "$fn" || { - echo "Test case '$fn' failed. Check the logs in .test/$fn/run.log" - global_exit_code=1 - } - echo "ok" - done + for fn in "${test_functions[@]}"; do + if "$fn"; then + echo "ok" + else + echo "Test case '$fn' failed. Check the logs in .test/$fn/run.log" + global_exit_code=1 + fi + done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@_tests.sh` around lines 447 - 453, The loop prints "ok" unconditionally after running each test, so failed tests also emit "ok"; update the loop in the test runner that iterates over test_functions so that the success message is only printed on success: run "$fn" and if it returns non-zero set global_exit_code=1 and print the failure message, otherwise print "ok" (i.e., move the echo "ok" into the success branch or use an if/then/else around "$fn"). Ensure references to test_functions, global_exit_code and the run.log path remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@_tests.sh`:
- Around line 457-460: The error message uses an undefined variable $fn causing
an unbound variable error; update the failure echo to reference the correct
variable $arg_testcase (i.e., change ".test/$fn/run.log" to
".test/$arg_testcase/run.log") so the message prints the real test case name and
then set global_exit_code=1 remains; locate this in the shell snippet around the
"$arg_testcase" execution branch and replace any $fn occurrences with
$arg_testcase.
- Line 441: The trap currently registered as trap 'echo "Script interrupted";
exit 130' INT TERM ERR EXIT HUP QUIT ABRT ALRM PIPE incorrectly includes EXIT
and ERR and thus forces exit 130 on every script termination; remove EXIT and
ERR from that trap so only asynchronous signals are handled (keep INT TERM HUP
QUIT ABRT ALRM PIPE) and ensure the handler remains echo "Script interrupted";
exit 130 for those signals, leaving normal exit code behavior intact for normal
exits and command errors.
---
Outside diff comments:
In `@_tests.sh`:
- Around line 447-453: The loop prints "ok" unconditionally after running each
test, so failed tests also emit "ok"; update the loop in the test runner that
iterates over test_functions so that the success message is only printed on
success: run "$fn" and if it returns non-zero set global_exit_code=1 and print
the failure message, otherwise print "ok" (i.e., move the echo "ok" into the
success branch or use an if/then/else around "$fn"). Ensure references to
test_functions, global_exit_code and the run.log path remain unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.test/10/git-reference.logis excluded by!**/*.log
📒 Files selected for processing (2)
_tests.shgit-artifact
There was a problem hiding this comment.
♻️ Duplicate comments (1)
_tests.sh (1)
454-457:⚠️ Potential issue | 🟠 MajorFix undefined variable in single-testcase failure message.
Line 455 and Line 455’s path use
$fn, which is undefined in this branch. Withset -u, this can mask the real failure path.🔧 Proposed fix
- "$arg_testcase" || { - echo "Test case '$fn' failed. Check the logs in .test/$fn/run.log" + "$arg_testcase" || { + echo "Test case '$arg_testcase' failed. Check the logs in .test/$arg_testcase/run.log" global_exit_code=1 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@_tests.sh` around lines 454 - 457, The failure message uses the undefined variable $fn which breaks under set -u; update the echo and path to reference the actual variable $arg_testcase (e.g., echo "Test case '$arg_testcase' failed. Check the logs in .test/$arg_testcase/run.log") and ensure the variable is quoted everywhere (".test/$arg_testcase/run.log") so the error branch prints the correct testcase name and path instead of referencing $fn.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@_tests.sh`:
- Around line 454-457: The failure message uses the undefined variable $fn which
breaks under set -u; update the echo and path to reference the actual variable
$arg_testcase (e.g., echo "Test case '$arg_testcase' failed. Check the logs in
.test/$arg_testcase/run.log") and ensure the variable is quoted everywhere
(".test/$arg_testcase/run.log") so the error branch prints the correct testcase
name and path instead of referencing $fn.
1 participant
with error message:
Summary by CodeRabbit
Bug Fixes
Tests